You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The explicit cast to string decodes Base64 using UTF-8. Verify this is correct for all expected response bodies (binary data or non-UTF-8 encodings may throw or corrupt data). Consider exposing a byte[] accessor or encoding-parameterized decode to avoid misuse.
publicstaticexplicitoperator string(BytesValuevalue)=>valueswitch{StringBytesValuestringBytesValue=>stringBytesValue.Value,Base64BytesValuebase64BytesValue=>System.Text.Encoding.UTF8.GetString(Convert.FromBase64String(base64BytesValue.Value)),
_ =>thrownewInvalidCastException($"Cannot cast '{value.GetType()}' to '{typeof(string)}'.")};
DisposeAsync calls RemoveAsync without try/catch and without ConfigureAwait. If removal fails (e.g., session closed), disposal could throw during using/await using. Consider swallowing/logging expected disposal-time errors to keep disposal idempotent and resilient.
Converter assumes JSON token is a non-null string and uses null-forgiving operator. Add validation to guard against null/invalid token types to avoid NullReferenceException at runtime.
Validate the JSON token type before reading to prevent invalid JSON from causing unexpected behavior. Throw a JsonException when the token is not a string or when the id is null/empty.
public override Collector? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
{
+ if (reader.TokenType != JsonTokenType.String)+ {+ throw new JsonException("Expected string token for Collector id.");+ }+
var id = reader.GetString();
+ if (string.IsNullOrEmpty(id))+ {+ throw new JsonException("Collector id cannot be null or empty.");+ }- return new Collector(_bidi, id!);+ return new Collector(_bidi, id);
}
Apply / Chat
Suggestion importance[1-10]: 8
__
Why: This suggestion significantly improves the robustness of the JSON converter by adding validation for the token type and for a null or empty id, preventing potential exceptions from malformed JSON.
Medium
Make dispose safe and idempotent
Dispose should be resilient and idempotent. Wrap the removal in a try/catch to avoid throwing during dispose paths and guard against double-dispose to prevent multiple removal attempts.
Why: The suggestion correctly implements the standard disposable pattern, making DisposeAsync idempotent and preventing exceptions from being thrown, which is a crucial best practice for resource management.
Medium
General
Add null-safe explicit cast
Handle null inputs defensively to avoid a NullReferenceException during explicit cast. Also use the Encoding property directly and consider malformed base64 data to avoid hard crashes.
-public static explicit operator string(BytesValue value) => value switch+public static explicit operator string?(BytesValue? value) => value switch
{
+ null => null,
StringBytesValue stringBytesValue => stringBytesValue.Value,
Base64BytesValue base64BytesValue => System.Text.Encoding.UTF8.GetString(Convert.FromBase64String(base64BytesValue.Value)),
_ => throw new InvalidCastException($"Cannot cast '{value.GetType()}' to '{typeof(string)}'.")
};
Apply / Chat
Suggestion importance[1-10]: 6
__
Why: The suggestion correctly improves the robustness of the explicit cast operator by handling null input, which aligns with standard C# casting behavior and prevents potential NullReferenceException.
Low
Learned best practice
Validate constructor parameters
Validate constructor arguments to prevent invalid state and clearer errors. Check for null bidi and null/empty id and throw ArgumentNullException or ArgumentException.
internal Collector(BiDi bidi, string id)
{
- _bidi = bidi;- Id = id;+ _bidi = bidi ?? throw new ArgumentNullException(nameof(bidi));+ Id = !string.IsNullOrEmpty(id) ? id : throw new ArgumentException("Collector id cannot be null or empty.", nameof(id));
}
Apply / Chat
Suggestion importance[1-10]: 6
__
Why:
Relevant best practice - Add null checks and validation for parameters before using them to prevent runtime errors.
The new explicit cast to string on BytesValue throws for non-string implementations, but there is no equivalent cast/accessor for Base64BytesValue. Consider adding a safe API (e.g., TryGetString/ToStringUnsafe or a byte[] accessor) to avoid misuse and clarify expectations.
publicstaticexplicitoperator string(BytesValuevalue){if(valueisStringBytesValuestringBytesValue){returnstringBytesValue.Value;}thrownewInvalidCastException($"Cannot cast '{value.GetType()}' to '{typeof(string)}'.");}
DefaultOptions no longer pins a specific Chrome BrowserVersion. This may increase test flakiness across environments. Validate that CI matrices remain stable or reintroduce pinning where necessary.
DisposeAsync awaits RemoveAsync but does not handle exceptions; disposal during shutdown might throw. Consider swallowing/logging specific errors or providing a cancellation/timeout to make disposal resilient.
In the Read method, add a null check for the id returned by reader.GetString() and throw a JsonException if it is null to prevent potential runtime errors.
public override Collector? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
{
var id = reader.GetString();
- return new Collector(_bidi, id!);+ if (id is null)+ {+ throw new JsonException("Collector ID cannot be null.");+ }++ return new Collector(_bidi, id);
}
Apply / Chat
Suggestion importance[1-10]: 7
__
Why: The suggestion correctly identifies that reader.GetString() can return null, and the use of the null-forgiving operator (!) could lead to a runtime exception. Adding a null check improves the robustness of the JSON deserialization logic.
Medium
General
Improve event handler robustness in test
To improve test robustness, replace SetResult with TrySetResult and wrap the logic in a try...catch block to safely handle exceptions on the TaskCompletionSource.
await using var _ = await bidi.Network.OnResponseCompletedAsync(async e =>
{
if (e.Response.Url.Contains("simpleTest.html"))
{
- responseBodyCompletionSource.SetResult((string)await bidi.Network.GetDataAsync(DataType.Response, e.Request.Request));+ try+ {+ var data = await bidi.Network.GetDataAsync(DataType.Response, e.Request.Request);+ responseBodyCompletionSource.TrySetResult((string)data);+ }+ catch (Exception ex)+ {+ responseBodyCompletionSource.TrySetException(ex);+ }
}
});
Apply / Chat
Suggestion importance[1-10]: 7
__
Why: The suggestion correctly identifies two potential issues in the test code: a race condition that could cause test flakiness and unhandled exceptions in an async event handler. The proposed changes make the test significantly more robust.
Medium
Learned best practice
Make disposal idempotent and safe
Guard against multiple DisposeAsync calls and avoid throwing if underlying removal fails; make disposal idempotent and resilient (e.g., track disposed state and swallow expected errors).
public static explicit operator string(BytesValue value)
{
+ if (value is null)+ {+ throw new ArgumentNullException(nameof(value));+ }
if (value is StringBytesValue stringBytesValue)
{
return stringBytesValue.Value;
}
-
throw new InvalidCastException($"Cannot cast '{value.GetType()}' to '{typeof(string)}'.");
}
Apply / Chat
Suggestion importance[1-10]: 5
__
Why:
Relevant best practice - Add precise validation and defensive checks around parsing and conversions to avoid null/invalid casts.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
https://w3c.github.io/webdriver-bidi/#command-network-addDataCollector
https://w3c.github.io/webdriver-bidi/#command-network-getData
https://w3c.github.io/webdriver-bidi/#command-network-removeDataCollector
🔄 Types of changes
PR Type
Enhancement
Description
Add network data collectors for BiDi protocol
Implement response body retrieval functionality
Support collector lifecycle management with disposal
Add JSON serialization for new network commands
Diagram Walkthrough
File Walkthrough
2 files
Register CollectorConverter for JSON serializationAdd JSON serialization attributes for data collector commands7 files
Create JSON converter for Collector objectsImplement add data collector command and parametersAdd explicit string conversion operatorCreate Collector class with disposal supportImplement get data command for retrieving response bodiesAdd data collector and get data methodsImplement remove data collector command1 files
Add tests for data collector functionality1 files
Remove hardcoded Chrome version constraint